Skip to content

feat: Add Latest State TransactionValidator implementation#1498

Merged
mattsse merged 16 commits intoparadigmxyz:mainfrom
chirag-bgh:feat/Add_Latest_State_TransactionValidator_implementation
Mar 10, 2023
Merged

feat: Add Latest State TransactionValidator implementation#1498
mattsse merged 16 commits intoparadigmxyz:mainfrom
chirag-bgh:feat/Add_Latest_State_TransactionValidator_implementation

Conversation

@chirag-bgh
Copy link
Copy Markdown
Contributor

@chirag-bgh chirag-bgh commented Feb 22, 2023 •
edited
Loading

Closes #1289

@chirag-bgh
Copy link
Copy Markdown
Contributor Author

chirag-bgh commented Feb 22, 2023 •
edited
Loading

@mattsse
I'm not able to implement all the checks for transaction validation.
Moreover, i need to change the return type for the fn validate_transaction in order to use ? operator and I think there's need to introduce more error fields in PoolError

mattsse
mattsse requested changes Feb 22, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgot about the error.
let's change the function so it returns a result instead.

Comment thread crates/transaction-pool/src/validate.rs Outdated
Comment thread crates/transaction-pool/src/validate.rs Outdated
@mattsse mattsse added the A-tx-pool Related to the transaction mempool label Feb 22, 2023
gakonst
gakonst reviewed Feb 24, 2023
View reviewed changes
Comment thread crates/transaction-pool/Cargo.toml Outdated
Comment thread crates/transaction-pool/src/validate.rs
@mattsse mattsse mentioned this pull request Mar 2, 2023
Merged
@mattsse
Copy link
Copy Markdown
Collaborator

mattsse commented Mar 3, 2023

@chirag-bgh this will be a blocker shortly, do you have bandwidth to get this over the line?

@chirag-bgh
Copy link
Copy Markdown
Contributor Author

chirag-bgh commented Mar 3, 2023

@chirag-bgh this will be a blocker shortly, do you have bandwidth to get this over the line?

I will get this done asap.

@mattsse mattsse mentioned this pull request Mar 3, 2023
Closed
1 task
@chirag-bgh
Copy link
Copy Markdown
Contributor Author

chirag-bgh commented Mar 3, 2023 •
edited
Loading

@mattsse
For chain_id, I suppose we need to use :

reth/crates/primitives/src/transaction/mod.rs

Lines 168 to 175 in a553c1d

pub enum Transaction {
/// Legacy transaction.
Legacy(TxLegacy),
/// Transaction with an [`AccessList`] ([EIP-2930](https://eips.ethereum.org/EIPS/eip-2930)).
Eip2930(TxEip2930),
/// A transaction with a priority fee ([EIP-1559](https://eips.ethereum.org/EIPS/eip-1559)).
Eip1559(TxEip1559),
}

But I'm not able to figure out how this should be done.

@mattsse
Copy link
Copy Markdown
Collaborator

mattsse commented Mar 3, 2023

right, we'd need to extend the PoolTransaction trait with

fn chain_id() -> Option<u64>

this is an option because it is optional for legacy tx

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 3, 2023 •
edited by codecov bot
Loading

Codecov Report

❌ Patch coverage is 8.77193% with 104 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.32%. Comparing base (9dfee49) to head (4c8ac33).
⚠️ Report is 12827 commits behind head on main.

Files with missing lines Patch % Lines
crates/transaction-pool/src/validate.rs 0.00% 84 Missing ⚠️
crates/transaction-pool/src/lib.rs 50.00% 7 Missing ⚠️
crates/interfaces/src/consensus.rs 0.00% 3 Missing ⚠️
crates/transaction-pool/src/test_utils/mock.rs 0.00% 3 Missing ⚠️
crates/transaction-pool/src/traits.rs 0.00% 3 Missing ⚠️
crates/rpc/rpc/src/eth/error.rs 0.00% 2 Missing ⚠️
crates/transaction-pool/src/error.rs 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1498      +/-   ##
==========================================
- Coverage   74.48%   74.32%   -0.17%     
==========================================
  Files         377      377              
  Lines       45197    45355     +158     
==========================================
+ Hits        33667    33711      +44     
- Misses      11530    11644     +114
Flag Coverage Δ
integration-tests 20.90% <8.77%> (-0.04%) ⬇️
unit-tests 68.91% <8.77%> (-0.18%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@chirag-bgh chirag-bgh marked this pull request as ready for review March 4, 2023 01:05
@chirag-bgh chirag-bgh requested a review from mattsse March 4, 2023 03:51
mattsse
mattsse requested changes Mar 4, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for this.

left a few comments,questions and suggestions.
this looks pretty good, just need to get all checks right.

we should have a closer look at how geth handles some of the checks as well.

Comment thread crates/transaction-pool/src/error.rs Outdated
Comment thread crates/transaction-pool/src/error.rs Outdated
Comment thread crates/transaction-pool/src/lib.rs Outdated
Comment thread crates/transaction-pool/src/traits.rs Outdated
Comment thread crates/transaction-pool/src/validate.rs
Comment thread crates/transaction-pool/src/validate.rs Outdated
Comment thread crates/transaction-pool/src/validate.rs
Comment thread crates/transaction-pool/src/validate.rs Outdated
Comment thread crates/transaction-pool/src/validate.rs
Comment thread crates/transaction-pool/Cargo.toml Outdated
@mattsse mattsse mentioned this pull request Mar 5, 2023
Merged
@chirag-bgh chirag-bgh requested a review from mattsse March 5, 2023 22:25
mattsse
mattsse requested changes Mar 6, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great progress,

I think we're almost there.
mostly nits.
the Arc change seems redundant?

Comment thread crates/transaction-pool/src/lib.rs Outdated
Comment thread crates/transaction-pool/src/validate.rs Outdated
Comment thread crates/transaction-pool/src/validate.rs Outdated
Comment thread crates/transaction-pool/src/validate.rs Outdated
Comment thread crates/transaction-pool/src/validate.rs Outdated
mattsse
mattsse requested changes Mar 7, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't mark requests as resolved without addressing them if no changes are made.

last change requests I think.

Comment thread crates/interfaces/src/consensus.rs
Comment thread crates/transaction-pool/src/lib.rs Outdated
Comment thread crates/transaction-pool/src/validate.rs Outdated
Comment thread crates/transaction-pool/src/validate.rs Outdated
Comment thread crates/transaction-pool/src/validate.rs Outdated
@chirag-bgh
Copy link
Copy Markdown
Contributor Author

chirag-bgh commented Mar 7, 2023 •
edited
Loading

please don't mark requests as resolved without addressing them if no changes are made.

last change requests I think.

Sorry, I marked them as resolved because I made the changes on my local and didn't pushed so that I could keep track of unresolved comments. Will do it now.

mattsse
mattsse approved these changes Mar 10, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merging to unblock, this PR has been going on for a bit too long.

@mattsse mattsse merged commit ba96b9d into paradigmxyz:main Mar 10, 2023
@mattsse mattsse mentioned this pull request Mar 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gakonst gakonst gakonst left review comments

@mattsse mattsse mattsse approved these changes

Assignees

No one assigned

Labels

A-tx-pool Related to the transaction mempool

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add Latest State TransactionValidator implementation

4 participants

@chirag-bgh @mattsse @codecov-commenter @gakonst